refactor(ramps): streamline RampsController reset and abort logic #7818
+284
−54
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
cursor review |
georgeweiler
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
georgeweiler
changed the title
| if (resource) { | ||
| (resource as Record<string, unknown>)[field] = value; | ||
| } | ||
| (state as Record<string, unknown>)[resourceType] = next; |
There was a problem hiding this comment.
Race condition in resource field updates can lose data
High Severity
The refactored #updateResourceField method reads this.state[resourceType] outside the update() callback, creates a new object with the updated field, then replaces the entire resource inside the callback. This creates a race condition: if another update() modifies any property of the resource (like data or selected) between capturing the snapshot and executing the callback, those changes are overwritten by the stale next object. The old implementation correctly mutated the draft state inside the callback, which is the proper Immer pattern.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Refactors region-dependent reset behavior into a single helper and improves abort handling so dependent resources stay in sync.
Changes
DEPENDENT_RESOURCE_KEYSandresetResource()so "reset one resource" is defined once and driven bygetDefaultRampsControllerState().resetDependentResources()to callgetDefaultRampsControllerState()once and loop over the keys instead of many manual assignments.resetResource(state, 'paymentMethods')insetSelectedProviderandsetSelectedTokenso payment methods are fully reset (includingisLoadinganderror), fixing stale loading/error state when changing or clearing provider/token.Draft<RampsControllerState>so they can be used insideupdate()without casts and avoid TS2589.Result
Single source of truth for default/reset state, less duplicated code, and correct clearing of
paymentMethods(and optionallyquotes.selected) on region or selection changes.References
Checklist
Note
Medium Risk
Touches
RampsControllerrequest lifecycle (caching/loading/error updates and abort races) and reset behavior for multiple region-dependent resources, which can affect UI state consistency under concurrency. Changes are well-covered by additional defensive tests but still impact core controller behavior.Overview
Refactors region-dependent state resets to a shared
resetResourcehelper driven bygetDefaultRampsControllerState(), and uses it to fully resetpaymentMethods(includingisLoading/error) when clearing or changing selected provider/token.Hardens request/resource bookkeeping:
PendingRequestnow tracksresourceType, resource field updates become defensive/immutable, andgetCountries()defensively stores[]when the service returns a non-array.Updates tests and changelog to cover the new reset behavior, hydration/refetch expectations, and timing/race-condition scenarios around dependent resources and loading/error state.
Written by Cursor Bugbot for commit 0a1775b. This will update automatically on new commits. Configure here.